Skip to content

Fix signed integer overflow with midpoint interpolation#107

Open
miikkas wants to merge 1 commit intorust-ndarray:masterfrom
miikkas:fix-midpoint-signed-overflow
Open

Fix signed integer overflow with midpoint interpolation#107
miikkas wants to merge 1 commit intorust-ndarray:masterfrom
miikkas:fix-midpoint-signed-overflow

Conversation

@miikkas
Copy link

@miikkas miikkas commented Oct 25, 2025 •
edited
Loading

I found this while exploring whether QuickCheck could be updated. Its newer version seems to run more thorough testing, which revealed this problem among others.

Assembly comparison

The new algorithm is not really much more complex than the original when comparing the assembly.

I checked the Compiler Explorer results for the following simplified versions, using -C opt-level=1 for compiler option:

#[unsafe(no_mangle)]
pub fn naive_midpoint_i64(lower: i64, higher: i64) -> i64 {
    lower + (higher - lower) / 2
}
#[unsafe(no_mangle)]
pub fn interpolate(lower: i64, higher: i64) -> i64 {
    let (lower_half, lower_rem) = (lower.div_euclid(2), lower.rem_euclid(2));
    let (higher_half, higher_rem) = (higher.div_euclid(2), higher.rem_euclid(2));
    lower_half + higher_half + (lower_rem * higher_rem)
}

The old algorithm produces the following assembly:

naive_midpoint_i64:
        sub     rsi, rdi
        mov     rax, rsi
        shr     rax, 63
        add     rax, rsi
        sar     rax
        add     rax, rdi
        ret

The new algorithm produces the following:

interpolate:
        mov     rcx, rdi
        sar     rcx
        and     edi, esi
        mov     rax, rsi
        sar     rax
        add     rax, rcx
        and     edi, 1
        add     rax, rdi
        ret

So it seems the new algorithm is two instructions longer.

I ran additional testing locally with the following command to ensure the changed algorithm returns the exact same results as the previous one:

QUICKCHECK_TESTS=100000000 cargo test --lib test_midpoint_algo_eq_naive_algo_i64

Something I'm a bit unsure about is whether there are any additional types Midpoint should be implemented for.

@miikkas
Fix signed overflow with midpoint interpolation
1aaa960 
The simple algorithm is replaced with an Euclid division and remainder
based slightly more complex one when the inputs are integers. Moreover,
the implementation is separated for integers and floating point numbers,
relying on macros instead of generics with trait bounds.

A regression test that failed before the changes is added along with
property-based testing ensuring that the results match those that the
previous version output (when within the non-overflowing limits).
@miikkas miikkas force-pushed the fix-midpoint-signed-overflow branch from 15307eb to 1aaa960 Compare January 27, 2026 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@miikkas